fix: use stability window for slot threshold #985
Conversation
📝 WalkthroughWalkthroughReplaces ad-hoc and hard-coded stability-window logic with an era-aware calculation by adding Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (9)📚 Learning: 2025-10-22T21:28:48.396ZApplied to files:
📚 Learning: 2025-10-22T20:13:35.164ZApplied to files:
📚 Learning: 2025-10-22T20:13:35.972ZApplied to files:
📚 Learning: 2025-10-22T21:29:26.924ZApplied to files:
📚 Learning: 2025-10-22T20:19:25.277ZApplied to files:
📚 Learning: 2025-10-22T21:28:48.396ZApplied to files:
📚 Learning: 2025-10-22T20:19:26.360ZApplied to files:
📚 Learning: 2025-10-22T20:18:45.125ZApplied to files:
📚 Learning: 2025-10-22T20:19:26.360ZApplied to files:
🧬 Code graph analysis (2)ledger/state.go (3)
ledger/chainsync_test.go (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ledger/chainsync.go (1)
274-314: Critical: Unreachable dead code and calculation issues.
Unreachable dead code: Lines 279-281 return early for Byron era (
Id == 0), making the Byron-specific branches at lines 300-301 and 307-308 unreachable. These conditions will never be true since we've already returned for Byron.Same calculation issues as handleEventChainsyncBlockHeader:
- Missing nil check for
ActiveSlotsCoeff.Rat(line 312)- Incorrect use of
.Num().Uint64()instead of proper integer division (lines 310-314)- Missing validation for negative
SecurityParamApply this diff:
// Calculate stability window byronGenesis := ls.config.CardanoNodeConfig.ByronGenesis() shelleyGenesis := ls.config.CardanoNodeConfig.ShelleyGenesis() if byronGenesis == nil || shelleyGenesis == nil { return nil, errors.New("could not get genesis config") } - var securityParam uint64 - // Use K parameter from Byron Genesis during Byron era, SecurityParam from Shelley Genesis for later eras - if ls.currentEra.Id == 0 { // Byron era ID is 0 - securityParam = uint64(byronGenesis.ProtocolConsts.K) - } else { - securityParam = uint64(shelleyGenesis.SecurityParam) - } - // Calculate stability window: 3k/f for Shelley+ eras, just k for Byron - var stabilityWindow uint64 - if ls.currentEra.Id == 0 { // Byron era - stabilityWindow = securityParam - } else { - stabilityWindow = new(big.Rat).Quo( - big.NewRat(int64(3*securityParam), 1), - shelleyGenesis.ActiveSlotsCoeff.Rat, - ).Num().Uint64() - } + // Note: We're already in Shelley+ era due to early return at line 279 + // Use SecurityParam from Shelley Genesis + k := shelleyGenesis.SecurityParam + if k < 0 { + return nil, fmt.Errorf("security param must be non-negative: %d", k) + } + securityParam := uint64(k) + + // Calculate stability window: 3k/f for Shelley+ eras + if shelleyGenesis.ActiveSlotsCoeff.Rat == nil { + return nil, errors.New("ActiveSlotsCoeff.Rat is nil") + } + result := new(big.Rat).Quo( + big.NewRat(int64(3*securityParam), 1), + shelleyGenesis.ActiveSlotsCoeff.Rat, + ) + // Proper integer division: divide numerator by denominator + stabilityWindow := new(big.Int).Div(result.Num(), result.Denom()).Uint64()
🧹 Nitpick comments (1)
ledger/chainsync.go (1)
143-171: Consider extracting stability window calculation to a helper function.The stability window calculation logic (era-aware security parameter retrieval + 3k/f formula for Shelley+) is duplicated between
handleEventChainsyncBlockHeaderandcalculateEpochNonce. This duplication increases maintenance burden and the risk of inconsistencies.Consider creating a helper method:
// calculateStabilityWindow returns the stability window based on the current era. // For Byron era, returns k. For Shelley+ eras, returns 3k/f. // Returns the default threshold if genesis data is unavailable or invalid. func (ls *LedgerState) calculateStabilityWindow() uint64 { byronGenesis := ls.config.CardanoNodeConfig.ByronGenesis() shelleyGenesis := ls.config.CardanoNodeConfig.ShelleyGenesis() if byronGenesis == nil || shelleyGenesis == nil { return blockfetchBatchSlotThresholdDefault } var securityParam uint64 if ls.currentEra.Id == 0 { // Byron era securityParam = uint64(byronGenesis.ProtocolConsts.K) return securityParam // Byron uses k directly } // Shelley+ eras k := shelleyGenesis.SecurityParam if k < 0 { ls.config.Logger.Warn("invalid negative security parameter", "k", k) return blockfetchBatchSlotThresholdDefault } securityParam = uint64(k) // Calculate 3k/f if shelleyGenesis.ActiveSlotsCoeff.Rat == nil { ls.config.Logger.Warn("ActiveSlotsCoeff.Rat is nil") return blockfetchBatchSlotThresholdDefault } result := new(big.Rat).Quo( big.NewRat(int64(3*securityParam), 1), shelleyGenesis.ActiveSlotsCoeff.Rat, ) return new(big.Int).Div(result.Num(), result.Denom()).Uint64() }Then use it in both locations:
slotThreshold := ls.calculateStabilityWindow()Also applies to: 298-314
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (1)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
- GitHub Check: go-test (1.24.x, ubuntu-latest)
🔇 Additional comments (2)
ledger/state.go (1)
597-623: LGTM! Era-aware security parameter retrieval is correctly implemented.The implementation properly handles era-specific security parameter sources with appropriate error handling and validation. The check for negative k before casting to uint64 (lines 616-621) is essential to prevent incorrect large values.
ledger/chainsync.go (1)
40-40: LGTM! Clear naming improvement.Renaming to
blockfetchBatchSlotThresholdDefaultbetter conveys that this is a fallback value when genesis data is unavailable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ledger/chainsync.go (1)
346-390: Same duplication and overflow issues as blockfetch threshold calculation.This code segment duplicates the stability window calculation from lines 148-213. Please see my earlier comment on those lines suggesting extraction into a shared helper function.
Additionally, line 379 has the same potential uint64 overflow issue as line 193: computing
3 * securityParambefore converting tobig.Intcould overflow ifsecurityParamis large enough.
🧹 Nitpick comments (2)
ledger/state.go (1)
597-632: Suggest extracting era-aware security parameter retrieval into a helper function.This security parameter retrieval logic (Byron K vs. Shelley SecurityParam with validation) is duplicated in
ledger/chainsync.goat lines 148-172 and 346-362. Consider extracting it into a helper method onLedgerState, such as:func (ls *LedgerState) getSecurityParam() (uint64, error) { if ls.currentEra.Id == 0 { // Byron era byronGenesis := ls.config.CardanoNodeConfig.ByronGenesis() if byronGenesis == nil { return 0, errors.New("failed to get Byron Genesis config") } k := byronGenesis.ProtocolConsts.K if k < 0 { return 0, fmt.Errorf("Byron K parameter must be non-negative: %d", k) } return uint64(k), nil // #nosec G115 } else { // Shelley and later eras shelleyGenesis := ls.config.CardanoNodeConfig.ShelleyGenesis() if shelleyGenesis == nil { return 0, errors.New("failed to get Shelley Genesis config") } k := shelleyGenesis.SecurityParam if k < 0 { return 0, fmt.Errorf("security param must be non-negative: %d", k) } return uint64(k), nil // #nosec G115 } }This would improve maintainability and ensure consistency across the codebase.
ledger/chainsync.go (1)
193-193: Apply defensive big.Int arithmetic for the security parameter multiplication to prevent theoretical uint64 overflow.While Cardano protocol specifies no hardcoded maximum for the security parameter k and it's governance-changeable via on-chain updates, the current code multiplies
3 * securityParamin uint64 before converting to big.Int (ledger/chainsync.go lines 193 and 379). IfsecurityParamever exceeds ~6.1e18, this wraps silently and corrupts the stability window calculation—a bug the subsequentIsUint64()check won't catch.Though mainnet k=2160 makes overflow practically impossible today, use big.Int throughout for robustness:
-numerator := new(big.Int).SetUint64(3 * securityParam) +numerator := new(big.Int).SetUint64(securityParam) +numerator.Mul(numerator, big.NewInt(3))Apply this fix at both line 193 and line 379 in ledger/chainsync.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (1)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)
ledger/chainsync.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: nilaway
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
ledger/chainsync.go (2)
39-40: LGTM!The rename from
blockfetchBatchSlotThresholdtoblockfetchBatchSlotThresholdDefaultclearly indicates this is a fallback value, which aligns well with the dynamic calculation logic introduced below.
215-219: LGTM!The computed
slotThresholdis correctly applied in the conditional to gate early return when the node is far from the upstream tip.
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
6734d13 to
8a15b9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ledger/state.go (1)
341-361: Avoid hard failure when Byron genesis is absent.
calculateEpochNoncealready returns early for Byron (Line 327), so deployments that start in Shelley legitimately provide no Byron genesis. The newif byronGenesis == nil || shelleyGenesis == nilcheck makes those setups bail out with “could not get genesis config”, breaking epoch rollover. Please only require the genesis file for the era you actually use here.Apply both hunks to keep compilation intact:
- byronGenesis := ls.config.CardanoNodeConfig.ByronGenesis() - shelleyGenesis := ls.config.CardanoNodeConfig.ShelleyGenesis() - if byronGenesis == nil || shelleyGenesis == nil { - return nil, errors.New("could not get genesis config") - } + shelleyGenesis := ls.config.CardanoNodeConfig.ShelleyGenesis() + if shelleyGenesis == nil { + return nil, errors.New("failed to get Shelley genesis config") + } @@ - if ls.currentEra.Id == 0 { // Byron era ID is 0 - k := byronGenesis.ProtocolConsts.K + if ls.currentEra.Id == 0 { // Byron era ID is 0 + byronGenesis := ls.config.CardanoNodeConfig.ByronGenesis() + if byronGenesis == nil { + return nil, errors.New("failed to get Byron genesis config") + } + k := byronGenesis.ProtocolConsts.K
🧹 Nitpick comments (1)
ledger/chainsync.go (1)
144-213: Factor stability-window calculation into a helper.This block duplicates the security-param extraction, ActiveSlotsCoeff validation, and overflow handling that
calculateEpochNonce(Lines 346-390) already performs. Consider extracting a sharedcalculateStabilityWindow()helper returning(uint64, error)so both call sites stay in sync—here you can warn and fall back on error, whilecalculateEpochNoncecan propagate it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (1)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)
ledger/chainsync.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: nilaway
- GitHub Check: lint
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
ledger/state.go (1)
598-632: LGTM: era-aware k-window guard.The non-negative checks and era split keep the cutoff aligned with the right security parameter without risking underflow.
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
8a15b9c to
e3e3f6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: lint
- GitHub Check: nilaway
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: Analyze (go)
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
e3e3f6a to
471d3d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
471d3d0 to
42827a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
wolf31o2
changed the title
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
42827a4 to
a2f1e1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ledger/state.go (1)
312-323: Ceil the Shelley-era stability window in epoch nonce calculationThe same floor-based division shows up here, so the epoch-nonce stability window can undershoot the spec by one slot on networks where
3*k/fis non-integral. That can change which block we treat as “before the stability window.” Please switch to the ceiling form as in the ledger reference.- numerator := new(big.Int).SetUint64(3 * securityParam) - quotient := new(big.Int).Div( - new(big.Int).Mul(numerator, activeSlotsRat.Denom()), - activeSlotsRat.Num(), - ) + numerator := new(big.Int).SetUint64(securityParam) + numerator.Mul(numerator, big.NewInt(3)) + numerator.Mul(numerator, activeSlotsRat.Denom()) + denominator := new(big.Int).Set(activeSlotsRat.Num()) + quotient, remainder := new(big.Int).QuoRem(numerator, denominator, new(big.Int)) + if remainder.Sign() != 0 { + quotient.Add(quotient, big.NewInt(1)) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
a2f1e1f to
594a6f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ledger/chainsync.go (1)
272-323: Consider calling calculateStabilityWindow() to reduce duplication.The stability window calculation logic here (lines 272-323) closely mirrors the implementation in
calculateStabilityWindow()(state.go lines 409-495). The only difference is error handling: this function returns errors whilecalculateStabilityWindow()logs and falls back to the default.While the past review comment suggested extracting this into a shared helper, the current implementation keeps them separate due to different error-handling semantics. Consider whether you could call
calculateStabilityWindow()and convert a zero return value into an error, which would eliminate the duplication while preserving the error-propagation behavior needed here.For example:
stabilityWindow := ls.calculateStabilityWindow() if stabilityWindow == blockfetchBatchSlotThresholdDefault { return nil, errors.New("could not calculate stability window from genesis config") }However, this approach loses granular error messages. If you prefer to keep the detailed validation errors, the current duplication is acceptable for maintainability of distinct error paths.
ledger/state.go (1)
685-720: Consider extracting era-aware security parameter retrieval into a helper.The era-aware security parameter logic (lines 685-720) is duplicated in
calculateEpochNonce(chainsync.go lines 272-295). Both branches check for genesis availability, validate non-negative parameters, and select between Byron K and Shelley SecurityParam based on era.Extracting a helper like
getSecurityParam() (uint64, error)would eliminate this duplication and ensure consistent validation across both call sites.For example:
func (ls *LedgerState) getSecurityParam() (uint64, error) { if ls.currentEra.Id == 0 { // Byron era byronGenesis := ls.config.CardanoNodeConfig.ByronGenesis() if byronGenesis == nil { return 0, errors.New("failed to get Byron Genesis config") } k := byronGenesis.ProtocolConsts.K if k <= 0 { return 0, fmt.Errorf("k parameter must be positive: %d", k) } return uint64(k), nil // #nosec G115 } // Shelley+ eras shelleyGenesis := ls.config.CardanoNodeConfig.ShelleyGenesis() if shelleyGenesis == nil { return 0, errors.New("failed to get Shelley Genesis config") } k := shelleyGenesis.SecurityParam if k <= 0 { return 0, fmt.Errorf("security param must be positive: %d", k) } return uint64(k), nil // #nosec G115 }This is optional since the duplication is limited and the code is clear as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/chainsync.go(3 hunks)ledger/state.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
ledger/chainsync.go (2)
39-40: LGTM: Constant rename clarifies fallback behavior.The rename from
blockfetchBatchSlotThresholdtoblockfetchBatchSlotThresholdDefaultaccurately reflects that this is now a fallback value when genesis data is unavailable or invalid.
143-146: LGTM: Dynamic stability window calculation properly integrated.The block header handling now uses the calculated stability window instead of a hard-coded value, achieving the PR's objective of protocol compliance and era awareness.
ledger/state.go (1)
409-495: LGTM: Comprehensive stability window calculation with proper safeguards.The
calculateStabilityWindow()method correctly implements:
- Era-aware parameter selection (Byron K vs. Shelley SecurityParam)
- Validation of genesis availability, non-negative/non-zero parameters, and ActiveSlotsCoeff
- Ceiling calculation for Shelley+ eras (3k/f rounded up) using proper big.Int arithmetic
- Overflow checks before uint64 conversion
- Fallback to default threshold on any validation failure
All issues raised in past review comments have been addressed.
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Creating a local commit for the generated unit tests... The unit tests have been committed to the current branch. Commit ID: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ledger/chainsync.go (1)
296-323: Consolidate stability window calculation
calculateEpochNoncere-implements the stability-window math even thoughhandleEventChainsyncBlockHeadernow relies onls.calculateStabilityWindow(). Keeping two hand-written copies makes it easy for the formulas or validations to drift apart the next time we tweak era handling or overflow checks. Please fold both call sites onto the same helper (or extract a shared internal routine that returns(uint64, error)socalculateEpochNoncecan surface hard failures) and have the caller decide whether to log/fallback or bubble the error. That keeps the protocol constants in one place and prevents future regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ledger/chainsync.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (1)
Rat(25-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
a43d73e to
3d97e2c
Compare
|
@coderabbitai the tests for this are failing |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12045 🏁 Script executed: Length of output: 4717 🏁 Script executed: Length of output: 889 🏁 Script executed: Length of output: 1649 🏁 Script executed: Length of output: 6322 🏁 Script executed: Length of output: 4114 🏁 Script executed: Length of output: 1216 🏁 Script executed: Length of output: 4951 @wolf31o2, I found the issue! The test The problem is that in Byron era (era ID 0), the The test expects an error but gets Here's the fix for the test: func TestCalculateEpochNonce_MissingByronGenesisInByronEra(t *testing.T) {
shelleyGenesisJSON := `{
"activeSlotsCoeff": 0.05,
"securityParam": 432,
"systemStart": "2022-10-25T00:00:00Z"
}`
cfg := &cardano.CardanoNodeConfig{
ShelleyGenesisHash: "363498d1024f84bb39d3fa9593ce391483cb40d479b87233f868d6e57c3a400d",
}
if err := cfg.LoadShelleyGenesisFromReader(strings.NewReader(shelleyGenesisJSON)); err != nil {
t.Fatalf("failed to load Shelley genesis: %v", err)
}
ls := &LedgerState{
currentEra: eras.ByronEraDesc,
currentEpoch: models.Epoch{
EpochId: 1,
StartSlot: 86400,
Nonce: []byte{0x01, 0x02, 0x03},
},
config: LedgerStateConfig{
CardanoNodeConfig: cfg,
Logger: slog.New(slog.NewJSONHandler(io.Discard, nil)),
},
db: &database.Database{},
}
// Byron era should return nil nonce with no error (Byron doesn't have epoch nonces)
// The function returns early before any genesis validation
nonce, err := ls.calculateEpochNonce(nil, 86400)
if err != nil {
t.Errorf("expected no error for Byron era, got: %v", err)
}
if nonce != nil {
t.Errorf("expected nil nonce for Byron era, got: %v", nonce)
}
}The fix changes the test to correctly verify that:
This aligns with the actual behavior of the code where Byron era returns immediately without needing any genesis data. 🧠 Learnings used |
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
3d97e2c to
ad2f39f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ledger/chainsync.go(3 hunks)ledger/chainsync_test.go(1 hunks)ledger/state.go(2 hunks)ledger/state_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ledger/chainsync_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/state_test.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/state_test.go
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/state_test.go
🧬 Code graph analysis (3)
ledger/chainsync.go (2)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
ledger/state_test.go (5)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/state.go (2)
LedgerState(78-102)LedgerStateConfig(58-68)ledger/eras/byron.go (1)
ByronEraDesc(24-28)ledger/eras/shelley.go (1)
ShelleyEraDesc(30-41)ledger/eras/eras.go (1)
EraDesc(23-35)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
ad2f39f to
4a59bd9
Compare
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
4a59bd9 to
3dccfa3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ledger/chainsync.go(3 hunks)ledger/chainsync_test.go(1 hunks)ledger/state.go(4 hunks)ledger/state_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ledger/chainsync_test.go
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.
Applied to files:
ledger/chainsync.go
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/state_test.go
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/state_test.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/state_test.go
📚 Learning: 2025-10-22T20:13:35.972Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T20:13:35.972Z
Learning: Unit tests under utxorpc should assert that models.Block.Decode() returns a github.com/blinklabs-io/gouroboros/ledger/common.Block, not rely on models.Block directly in protocol/ledger-facing logic.
Applied to files:
ledger/state_test.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.
Applied to files:
ledger/state_test.go
🧬 Code graph analysis (2)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
ledger/state_test.go (9)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/state.go (2)
LedgerState(78-102)LedgerStateConfig(58-68)ledger/eras/byron.go (1)
ByronEraDesc(24-28)ledger/eras/shelley.go (1)
ShelleyEraDesc(30-41)ledger/eras/allegra.go (1)
AllegraEraDesc(30-41)ledger/eras/mary.go (1)
MaryEraDesc(30-41)ledger/eras/alonzo.go (1)
AlonzoEraDesc(30-41)ledger/eras/babbage.go (1)
BabbageEraDesc(30-41)ledger/eras/conway.go (1)
ConwayEraDesc(33-45)
🪛 GitHub Actions: go-test
ledger/chainsync.go
[error] 278-278: ledgerState.calculateEpochNonce panicked due to nil Shelley genesis during TestCalculateEpochNonce_MissingShelleyGenesis.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: nilaway
- GitHub Check: lint
- GitHub Check: Analyze (go)
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
294d4ef to
23e4983
Compare
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
23e4983 to
ac1e8c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ledger/chainsync.go(3 hunks)ledger/chainsync_test.go(1 hunks)ledger/state.go(4 hunks)ledger/state_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ledger/chainsync.go
- ledger/state_test.go
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:13:35.972Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T20:13:35.972Z
Learning: Unit tests under utxorpc should assert that models.Block.Decode() returns a github.com/blinklabs-io/gouroboros/ledger/common.Block, not rely on models.Block directly in protocol/ledger-facing logic.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:18:45.125Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T20:18:45.125Z
Learning: In tests, Chain.AddBlock must be given a gouroboros ledger.Block (not database/models.Block). Use a lightweight MockBlock (as in chain/chain_test.go) or call models.Block.Decode() when starting from stored rows.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: utxorpc tests must pass github.com/blinklabs-io/gouroboros/ledger/common.Block to chain.Chain.AddBlock. They should create database/models.Block only as a storage row, then call models.Block.Decode() to obtain a ledger/common.Block for AddBlock and any ledger/protocol-facing logic.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: chain.Chain.AddBlock expects a github.com/blinklabs-io/gouroboros/ledger.Block. In tests, construct a database/models.Block only as a container and call its Decode() to obtain the gouroboros ledger.Block (which also satisfies github.com/blinklabs-io/gouroboros/ledger/common.Block) before inserting into the chain.
Applied to files:
ledger/chainsync_test.go
🧬 Code graph analysis (2)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
ledger/chainsync_test.go (6)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/state.go (2)
LedgerState(78-102)LedgerStateConfig(58-68)ledger/eras/byron.go (1)
ByronEraDesc(24-28)ledger/eras/shelley.go (1)
ShelleyEraDesc(30-41)ledger/eras/eras.go (1)
EraDesc(23-35)database/database.go (1)
Database(45-50)
🪛 GitHub Actions: go-test
ledger/chainsync_test.go
[error] 208-208: TestCalculateEpochNonce_MissingShelleyGenesis failed with panic: runtime error: invalid memory address or nil pointer dereference
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
- GitHub Check: lint
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
ac1e8c9 to
fe8d7d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
ledger/state.go (1)
420-437: Apply fix to Byron stability window: use 2k, not k per Ouroboros specification.Byron's stability window specification is 2k slots per Ouroboros consensus documentation. The current implementation returns k directly (line 436), contradicting the spec. Tests currently validate the incorrect behavior (expecting k=2160 instead of 2k=4320).
Apply this fix:
// Byron uses k directly - return uint64(k) // #nosec G115 + return uint64(k) * 2 // Byron stability window is 2k slotsAlso update the function comment (line 410) to reflect the corrected behavior:
-// For Byron era, returns k. For Shelley+ eras, returns 3k/f. +// For Byron era, returns 2k. For Shelley+ eras, returns 3k/f.Update test expectations in ledger/state_test.go to validate 2k instead of k.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ledger/chainsync.go(3 hunks)ledger/chainsync_test.go(1 hunks)ledger/state.go(4 hunks)ledger/state_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/state_test.goledger/chainsync_test.goledger/state.go
📚 Learning: 2025-10-22T20:13:35.972Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T20:13:35.972Z
Learning: Unit tests under utxorpc should assert that models.Block.Decode() returns a github.com/blinklabs-io/gouroboros/ledger/common.Block, not rely on models.Block directly in protocol/ledger-facing logic.
Applied to files:
ledger/state_test.goledger/chainsync_test.goledger/state.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.
Applied to files:
ledger/state_test.goledger/chainsync_test.goledger/state.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.
Applied to files:
ledger/chainsync_test.goledger/state.goledger/chainsync.go
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/chainsync_test.goledger/state.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/chainsync_test.goledger/state.go
📚 Learning: 2025-10-22T20:18:45.125Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T20:18:45.125Z
Learning: In tests, Chain.AddBlock must be given a gouroboros ledger.Block (not database/models.Block). Use a lightweight MockBlock (as in chain/chain_test.go) or call models.Block.Decode() when starting from stored rows.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: utxorpc tests must pass github.com/blinklabs-io/gouroboros/ledger/common.Block to chain.Chain.AddBlock. They should create database/models.Block only as a storage row, then call models.Block.Decode() to obtain a ledger/common.Block for AddBlock and any ledger/protocol-facing logic.
Applied to files:
ledger/chainsync_test.goledger/state.go
🧬 Code graph analysis (3)
ledger/state_test.go (5)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/state.go (2)
LedgerState(78-102)LedgerStateConfig(58-68)ledger/eras/byron.go (1)
ByronEraDesc(24-28)ledger/eras/shelley.go (1)
ShelleyEraDesc(30-41)ledger/eras/eras.go (1)
EraDesc(23-35)
ledger/chainsync_test.go (11)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/state.go (2)
LedgerState(78-102)LedgerStateConfig(58-68)ledger/eras/byron.go (1)
ByronEraDesc(24-28)ledger/eras/shelley.go (1)
ShelleyEraDesc(30-41)ledger/eras/eras.go (1)
EraDesc(23-35)ledger/eras/allegra.go (1)
AllegraEraDesc(30-41)ledger/eras/mary.go (1)
MaryEraDesc(30-41)ledger/eras/alonzo.go (1)
AlonzoEraDesc(30-41)ledger/eras/babbage.go (1)
BabbageEraDesc(30-41)ledger/eras/conway.go (1)
ConwayEraDesc(33-45)database/database.go (1)
Database(45-50)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
🪛 GitHub Actions: go-test
ledger/chainsync_test.go
[error] 210-210: TestCalculateEpochNonce_MissingShelleyGenesis: expected error for missing Shelley genesis, got nil
[error] 379-379: TestCalculateEpochNonce_ZeroActiveSlots: panic caused by runtime error: nil pointer dereference during epoch nonce calculation
ledger/chainsync.go
[error] 278-278: ledger state calculation failed due to nil pointer dereference when accessing block/txn (BlockBeforeSlotTxn called with nil Txn).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
ledger/chainsync.go (2)
38-39: LGTM! Dynamic stability window successfully integrated.The replacement of the hard-coded threshold with a dynamic per-era calculation via
calculateStabilityWindow()achieves the PR objective of using protocol-defined stability windows.Also applies to: 143-143, 145-145
269-276: Verify transaction parameter in test callers.The stability window calculation correctly uses
calculateStabilityWindow()as suggested in past reviews. However, whencurrentEpoch.Nonceis non-empty, this code path requires database access viaBlockBeforeSlotTxn(txn, ...)at line 278.Tests calling
calculateEpochNonce(nil, ...)with a non-empty nonce will panic with a nil pointer dereference, which matches the pipeline failure. The test setup issue should be addressed in the test file.Production code is correct; the issue is in test setup (see comments on ledger/chainsync_test.go).
Also applies to: 278-281
ledger/state_test.go (1)
233-259: LGTM! Test correctly expects Byron security parameter.This test was correctly updated per past review feedback (commit 3dccfa3) to expect the Byron security parameter (432) rather than the default threshold when Shelley genesis is missing but Byron genesis is present during Byron era.
Based on learnings.
ledger/state.go (1)
439-503: LGTM! Shelley+ stability window calculation is robust.The implementation correctly:
- Validates Shelley genesis presence
- Checks for non-positive security parameter
- Guards against nil/non-positive ActiveSlotsCoeff
- Uses ceiling arithmetic (
QuoRem+ rounding up) for3k/f- Validates the result fits in
uint64- Falls back to default threshold on any validation failure
All issues raised in past reviews have been addressed.
| func TestCalculateEpochNonce_ZeroActiveSlots(t *testing.T) { | ||
| byronGenesisJSON := `{ | ||
| "protocolConsts": { | ||
| "k": 432, | ||
| "protocolMagic": 2 | ||
| } | ||
| }` | ||
| shelleyGenesisJSON := `{ | ||
| "activeSlotsCoeff": 0, | ||
| "securityParam": 432, | ||
| "systemStart": "2022-10-25T00:00:00Z" | ||
| }` | ||
|
|
||
| cfg := &cardano.CardanoNodeConfig{ | ||
| ShelleyGenesisHash: "363498d1024f84bb39d3fa9593ce391483cb40d479b87233f868d6e57c3a400d", | ||
| } | ||
| if err := cfg.LoadByronGenesisFromReader(strings.NewReader(byronGenesisJSON)); err != nil { | ||
| t.Fatalf("failed to load Byron genesis: %v", err) | ||
| } | ||
| if err := cfg.LoadShelleyGenesisFromReader(strings.NewReader(shelleyGenesisJSON)); err != nil { | ||
| t.Fatalf("failed to load Shelley genesis: %v", err) | ||
| } | ||
|
|
||
| ls := &LedgerState{ | ||
| currentEra: eras.ShelleyEraDesc, | ||
| currentEpoch: models.Epoch{ | ||
| EpochId: 1, | ||
| StartSlot: 86400, | ||
| Nonce: []byte{0x01, 0x02, 0x03}, | ||
| }, | ||
| config: LedgerStateConfig{ | ||
| CardanoNodeConfig: cfg, | ||
| Logger: slog.New(slog.NewJSONHandler(io.Discard, nil)), | ||
| }, | ||
| } | ||
|
|
||
| // Zero active slots coefficient should cause division by zero error | ||
| _, err := ls.calculateEpochNonce(nil, 86400) | ||
| if err == nil { | ||
| t.Error("expected error for zero active slots coefficient, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "divide by zero") && | ||
| !strings.Contains(err.Error(), "ActiveSlotsCoeff") { | ||
| t.Errorf( | ||
| "expected error about division by zero or ActiveSlotsCoeff, got: %v", | ||
| err, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix test: provide valid transaction or adjust test setup.
This test panics because it passes nil for the txn parameter (line 379) but sets a non-empty Nonce (line 370). With a non-empty nonce, calculateEpochNonce skips the initial epoch path and attempts database queries via BlockBeforeSlotTxn(txn, ...), causing a nil pointer dereference.
To fix, either:
- Preferred: Create a test database and pass a valid transaction:
+ db := database.NewTestDatabase(t) // or appropriate test DB setup
+ ls.db = db
+ txn := db.Transaction(false)
+ err := txn.Do(func(txn *database.Txn) error {
- _, err := ls.calculateEpochNonce(nil, 86400)
+ _, err := ls.calculateEpochNonce(txn, 86400)
+ return err
+ })
- if err == nil {
+ if err == nil || txn.Err() == nil {
t.Error("expected error for zero active slots coefficient, got nil")
}- Alternative: Set
Noncetonilso the test takes the initial epoch path that doesn't require database access (but then it won't test the stability window calculation).
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: go-test
[error] 379-379: TestCalculateEpochNonce_ZeroActiveSlots: panic caused by runtime error: nil pointer dereference during epoch nonce calculation
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
fe8d7d7 to
d93f575
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ledger/chainsync.go(4 hunks)ledger/chainsync_test.go(1 hunks)ledger/state.go(4 hunks)ledger/state_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.
Applied to files:
ledger/state.goledger/chainsync_test.go
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/state.goledger/chainsync_test.goledger/state_test.go
📚 Learning: 2025-10-22T20:13:35.972Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T20:13:35.972Z
Learning: Unit tests under utxorpc should assert that models.Block.Decode() returns a github.com/blinklabs-io/gouroboros/ledger/common.Block, not rely on models.Block directly in protocol/ledger-facing logic.
Applied to files:
ledger/state.goledger/chainsync_test.goledger/state_test.go
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/state.goledger/chainsync_test.goledger/state_test.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/state.goledger/chainsync_test.goledger/state_test.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: utxorpc tests must pass github.com/blinklabs-io/gouroboros/ledger/common.Block to chain.Chain.AddBlock. They should create database/models.Block only as a storage row, then call models.Block.Decode() to obtain a ledger/common.Block for AddBlock and any ledger/protocol-facing logic.
Applied to files:
ledger/state.goledger/chainsync_test.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.
Applied to files:
ledger/state.goledger/chainsync_test.goledger/state_test.go
📚 Learning: 2025-10-22T20:18:45.125Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T20:18:45.125Z
Learning: In tests, Chain.AddBlock must be given a gouroboros ledger.Block (not database/models.Block). Use a lightweight MockBlock (as in chain/chain_test.go) or call models.Block.Decode() when starting from stored rows.
Applied to files:
ledger/chainsync_test.go
🧬 Code graph analysis (4)
ledger/chainsync.go (1)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (2)
Rat(25-27)Uint64(54-54)
ledger/chainsync_test.go (6)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/state.go (2)
LedgerState(78-102)LedgerStateConfig(58-68)ledger/eras/byron.go (1)
ByronEraDesc(24-28)ledger/eras/shelley.go (1)
ShelleyEraDesc(30-41)ledger/eras/eras.go (1)
EraDesc(23-35)database/database.go (1)
Database(45-50)
ledger/state_test.go (10)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/state.go (2)
LedgerState(78-102)LedgerStateConfig(58-68)ledger/eras/byron.go (1)
ByronEraDesc(24-28)ledger/eras/shelley.go (1)
ShelleyEraDesc(30-41)ledger/eras/eras.go (1)
EraDesc(23-35)ledger/eras/allegra.go (1)
AllegraEraDesc(30-41)ledger/eras/mary.go (1)
MaryEraDesc(30-41)ledger/eras/alonzo.go (1)
AlonzoEraDesc(30-41)ledger/eras/babbage.go (1)
BabbageEraDesc(30-41)ledger/eras/conway.go (1)
ConwayEraDesc(33-45)
🪛 GitHub Actions: go-test
ledger/chainsync_test.go
[error] 372-372: TestCalculateEpochNonce_ZeroActiveSlots: expected error for zero active slots coefficient, got nil
[error] 374-374: panic: runtime error: invalid memory address or nil pointer dereference
🪛 GitHub Actions: golangci-lint
ledger/state.go
[error] 436-436: File is not properly formatted (gci).
🪛 GitHub Check: lint
ledger/state.go
[failure] 436-436:
File is not properly formatted (gci)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: nilaway
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
47b1ad0 to
9bb8bc1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ledger/chainsync_test.go (3)
217-257: Strengthen test assertions or clarify purpose.This test has weak assertions and doesn't effectively validate negative security parameter handling:
- Genesis loading errors are ignored (lines 234-235), so if negative k causes loading to fail, the test doesn't detect it.
- Byron era returns
(nil, nil)immediately regardless of genesis validity, socalculateEpochNoncenever validates the negative k.- The only assertion is a log statement when no error occurs (line 255).
Consider either:
- If testing genesis loading: Assert that
LoadByronGenesisFromReaderreturns an error for negative k.- If testing calculateEpochNonce: Use Shelley era with appropriate setup to exercise the validation path.
- If negative k is actually valid: Document why and what behavior is expected.
Apply this diff to test genesis loading validation:
- _ = cfg.LoadByronGenesisFromReader(strings.NewReader(byronGenesisJSON)) - _ = cfg.LoadShelleyGenesisFromReader(strings.NewReader(shelleyGenesisJSON)) - - ls := &LedgerState{ - currentEra: eras.ByronEraDesc, - currentEpoch: models.Epoch{ - EpochId: 1, - StartSlot: 86400, - Nonce: []byte{0x01, 0x02, 0x03}, - }, - config: LedgerStateConfig{ - CardanoNodeConfig: cfg, - Logger: slog.New(slog.NewJSONHandler(io.Discard, nil)), - }, - } - - // Test will depend on whether the genesis loads successfully - // If it loads, we expect an error about negative k - _, err := ls.calculateEpochNonce(nil, 86400) - // Either genesis loading fails or calculateEpochNonce catches negative k - if err == nil { - t.Log("Note: negative k may be caught during genesis loading") - } + err := cfg.LoadByronGenesisFromReader(strings.NewReader(byronGenesisJSON)) + if err == nil { + t.Error("expected error when loading Byron genesis with negative k") + } + // Alternatively, if negative k is allowed, document the expected behavior
523-569: Consider clarifying test name to match implementation.The test correctly validates that
calculateStabilityWindow()returns the protocol-derived value (25920) instead of the hardcoded constant. However, the test name suggests it testshandleEventChainsyncBlockHeaderbut only directly callscalculateStabilityWindow(). Consider renaming toTestCalculateStabilityWindow_ShelleyErafor clarity, or add a comment explaining that this validates the value used by the handler.
684-721: LGTM: Byron era early return correctly validated.The test properly verifies that Byron era returns nil immediately without validating genesis, as corrected in the referenced commit. The non-nil nonce (line 704) and database (line 710) setup are unused due to the early return but don't cause issues.
Optional cleanup to simplify the test:
ls := &LedgerState{ currentEra: eras.ByronEraDesc, currentEpoch: models.Epoch{ EpochId: 1, StartSlot: 86400, - Nonce: []byte{0x01, 0x02, 0x03}, + Nonce: nil, }, config: LedgerStateConfig{ CardanoNodeConfig: cfg, Logger: slog.New(slog.NewJSONHandler(io.Discard, nil)), }, - db: &database.Database{}, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ledger/chainsync_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:13:35.972Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T20:13:35.972Z
Learning: Unit tests under utxorpc should assert that models.Block.Decode() returns a github.com/blinklabs-io/gouroboros/ledger/common.Block, not rely on models.Block directly in protocol/ledger-facing logic.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:18:45.125Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T20:18:45.125Z
Learning: In tests, Chain.AddBlock must be given a gouroboros ledger.Block (not database/models.Block). Use a lightweight MockBlock (as in chain/chain_test.go) or call models.Block.Decode() when starting from stored rows.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: utxorpc tests must pass github.com/blinklabs-io/gouroboros/ledger/common.Block to chain.Chain.AddBlock. They should create database/models.Block only as a storage row, then call models.Block.Decode() to obtain a ledger/common.Block for AddBlock and any ledger/protocol-facing logic.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: chain.Chain.AddBlock expects a github.com/blinklabs-io/gouroboros/ledger.Block. In tests, construct a database/models.Block only as a container and call its Decode() to obtain the gouroboros ledger.Block (which also satisfies github.com/blinklabs-io/gouroboros/ledger/common.Block) before inserting into the chain.
Applied to files:
ledger/chainsync_test.go
🧬 Code graph analysis (1)
ledger/chainsync_test.go (6)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/state.go (2)
LedgerState(78-102)LedgerStateConfig(58-68)ledger/eras/byron.go (1)
ByronEraDesc(24-28)ledger/eras/shelley.go (1)
ShelleyEraDesc(30-41)ledger/eras/eras.go (1)
EraDesc(23-35)database/database.go (1)
Database(45-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: nilaway
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
ledger/chainsync_test.go (9)
32-77: LGTM: Byron era test correctly validates early return behavior.The test properly validates that Byron era returns nil nonce without error, which aligns with the early return in
calculateEpochNonce.
79-145: LGTM: Initial epoch test properly validates genesis hash return.The test correctly verifies that an initial Shelley epoch (nil nonce) returns the Shelley genesis hash, with thorough byte-by-byte validation.
147-189: LGTM: Invalid genesis hash test correctly expects error.The test properly validates that an invalid hex string in the genesis hash produces an error during epoch nonce calculation.
191-215: LGTM: Missing genesis test correctly validates error handling.The test properly validates that missing Shelley genesis hash produces an appropriate error, as fixed in the referenced commit.
259-339: LGTM: Comprehensive parameter variation coverage.The table-driven test effectively validates epoch nonce calculation across different security parameters and active slots coefficients, covering testnet, mainnet, and high-activity scenarios.
341-374: LGTM: Zero active slots test correctly validates fallback behavior.The test properly verifies that
calculateStabilityWindow()returns the fallback threshold whenActiveSlotsCoeffis zero, avoiding the nil transaction issue from previous iterations.
376-473: LGTM: Stability window calculation test with clear formula documentation.The test effectively validates stability window calculations across eras with well-documented expected formulas, providing good traceability for the 3k/f calculation.
475-521: LGTM: Integer arithmetic precision test covers fractional coefficient edge case.The test properly validates that the implementation handles fractional active slots coefficients correctly using integer arithmetic, catching potential rounding or precision issues.
571-682: LGTM: Comprehensive cross-era validation.The test provides excellent coverage across all Cardano eras (Byron through Conway), validating the era-aware behavior that's central to this PR's objectives.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
ledger/state_test.go (1)
494-498: Fix Byron era expectation and comment.The Byron era test case expects k (432) but the implementation returns 2k (864). The comment "Just k" is incorrect—Byron uses 2k per the consensus specification and the implementation at line 436 in state.go.
{ name: "Byron era", era: eras.ByronEraDesc, - expectedWindow: 432, // Just k + expectedWindow: 864, // Byron uses 2k },
🧹 Nitpick comments (1)
ledger/state.go (1)
435-435: Clean up redundant comment.The comment has "// Byron //" which appears to be a duplicate or merge artifact. Remove the redundant "Byron" text.
- // Byron // Byron stability window is 2k slots + // Byron stability window is 2k slots return uint64(k)*2 // #nosec G115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ledger/chainsync.go(4 hunks)ledger/chainsync_test.go(1 hunks)ledger/state.go(4 hunks)ledger/state_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ledger/chainsync.go
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/state_test.goledger/state.goledger/chainsync_test.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/state_test.goledger/state.goledger/chainsync_test.go
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/state_test.goledger/state.goledger/chainsync_test.go
📚 Learning: 2025-10-22T20:13:35.972Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T20:13:35.972Z
Learning: Unit tests under utxorpc should assert that models.Block.Decode() returns a github.com/blinklabs-io/gouroboros/ledger/common.Block, not rely on models.Block directly in protocol/ledger-facing logic.
Applied to files:
ledger/state_test.goledger/state.goledger/chainsync_test.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.
Applied to files:
ledger/state_test.goledger/state.goledger/chainsync_test.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.
Applied to files:
ledger/state.goledger/chainsync_test.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: utxorpc tests must pass github.com/blinklabs-io/gouroboros/ledger/common.Block to chain.Chain.AddBlock. They should create database/models.Block only as a storage row, then call models.Block.Decode() to obtain a ledger/common.Block for AddBlock and any ledger/protocol-facing logic.
Applied to files:
ledger/state.goledger/chainsync_test.go
📚 Learning: 2025-10-22T20:18:45.125Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T20:18:45.125Z
Learning: In tests, Chain.AddBlock must be given a gouroboros ledger.Block (not database/models.Block). Use a lightweight MockBlock (as in chain/chain_test.go) or call models.Block.Decode() when starting from stored rows.
Applied to files:
ledger/chainsync_test.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: chain.Chain.AddBlock expects a github.com/blinklabs-io/gouroboros/ledger.Block. In tests, construct a database/models.Block only as a container and call its Decode() to obtain the gouroboros ledger.Block (which also satisfies github.com/blinklabs-io/gouroboros/ledger/common.Block) before inserting into the chain.
Applied to files:
ledger/chainsync_test.go
🧬 Code graph analysis (3)
ledger/state_test.go (10)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/state.go (2)
LedgerState(78-102)LedgerStateConfig(58-68)ledger/eras/byron.go (1)
ByronEraDesc(24-28)ledger/eras/shelley.go (1)
ShelleyEraDesc(30-41)ledger/eras/eras.go (1)
EraDesc(23-35)ledger/eras/allegra.go (1)
AllegraEraDesc(30-41)ledger/eras/mary.go (1)
MaryEraDesc(30-41)ledger/eras/alonzo.go (1)
AlonzoEraDesc(30-41)ledger/eras/babbage.go (1)
BabbageEraDesc(30-41)ledger/eras/conway.go (1)
ConwayEraDesc(33-45)
ledger/state.go (3)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)database/plugin/log.go (1)
Logger(18-27)database/types/types.go (1)
Rat(25-27)
ledger/chainsync_test.go (4)
config/cardano/node.go (1)
CardanoNodeConfig(31-45)ledger/state.go (2)
LedgerState(78-102)LedgerStateConfig(58-68)ledger/eras/eras.go (1)
EraDesc(23-35)database/database.go (1)
Database(45-50)
🪛 GitHub Actions: go-test
ledger/state_test.go
[error] 93-93: TestCalculateStabilityWindow_ByronEra/Byron_era_with_k=1: expected stability window 1, got 2
[error] 257-257: TestCalculateStabilityWindow_EdgeCases/Missing_Shelley_genesis_returns_default: expected default threshold 432, got 864
[error] 545-545: TestCalculateStabilityWindow_AllEras/Byron_era: era Byron: expected stability window 432, got 864
[error] 592-592: TestCalculateStabilityWindow_Integration/Mainnet-like_configuration: Byron era: expected stability window 2160, got 4320
🪛 GitHub Actions: golangci-lint
ledger/state.go
[error] 436-436: golangci-lint: File is not properly formatted (gci)
🪛 GitHub Check: lint
ledger/state.go
[failure] 436-436:
File is not properly formatted (gci)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
ledger/chainsync_test.go (1)
1-721: Excellent test coverage for epoch nonce calculations.This test suite provides comprehensive coverage of epoch nonce calculation logic across all Cardano eras (Byron through Conway), including edge cases for missing or invalid genesis data, zero parameters, and precision handling. The tests are well-organized and clearly demonstrate the expected behavior for each scenario.
ledger/state.go (1)
436-436: Review comment is incorrect—gci only checks import formatting.Line 436 contains
return uint64(k)*2 // #nosec G115, which is a return statement in function code. The gci formatter only validates import block ordering and grouping; it does not check executable code statements. This review comment cannot be referring to gci formatting issues on that line.Likely an incorrect or invalid review comment.
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
9bb8bc1 to
8825574
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
ledger/state_test.go (3)
47-50: Fix Byron stability window expectation for k=1.The test expects 1 but
calculateStabilityWindow()returns2*kfor Byron era (line 436 in state.go). Update the expected value to 2.Apply this diff:
{ name: "Byron era with k=1", k: 1, - expectedWindow: 1, + expectedWindow: 2, },
233-259: Fix Byron stability window expectation when Shelley genesis is missing.This test expects 432 (k) when only Byron genesis is present, but
calculateStabilityWindow()returns2*kfor Byron era (line 436 in state.go). With k=432, the result is 864.Apply this diff:
result := ls.calculateStabilityWindow() - if result != 432 { - t.Errorf("expected default threshold %d, got %d", 432, result) + if result != 864 { + t.Errorf("expected Byron stability window (2*k) %d, got %d", 864, result) }
579-596: Fix mainnet Byron stability window expectation.The test expects 2160 (k) for mainnet Byron parameters, but the implementation returns
2*k. With k=2160, the Byron stability window is 4320 slots.Apply this diff:
resultByron := lsByron.calculateStabilityWindow() - if resultByron != 2160 { + if resultByron != 4320 { t.Errorf( - "Byron era: expected stability window 2160, got %d", + "Byron era: expected stability window 4320 (2*k), got %d", resultByron, ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ledger/chainsync.go(4 hunks)ledger/chainsync_test.go(1 hunks)ledger/state.go(4 hunks)ledger/state_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.
Applied to files:
ledger/state.goledger/chainsync_test.go
📚 Learning: 2025-10-22T20:13:35.164Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:56-56
Timestamp: 2025-10-22T20:13:35.164Z
Learning: In tests under ledger/state_test.go, PrimaryChain.AddBlock expects a "github.com/blinklabs-io/gouroboros/ledger/common".Block; use models.Block.Decode() to obtain that type before calling AddBlock.
Applied to files:
ledger/state.goledger/chainsync_test.goledger/state_test.go
📚 Learning: 2025-10-22T20:13:35.972Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T20:13:35.972Z
Learning: Unit tests under utxorpc should assert that models.Block.Decode() returns a github.com/blinklabs-io/gouroboros/ledger/common.Block, not rely on models.Block directly in protocol/ledger-facing logic.
Applied to files:
ledger/state.goledger/chainsync_test.goledger/state_test.go
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().
Applied to files:
ledger/state.goledger/chainsync_test.goledger/state_test.go
📚 Learning: 2025-10-22T20:19:25.277Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:434-434
Timestamp: 2025-10-22T20:19:25.277Z
Learning: In this repo’s tests, use models.Block.Decode() to obtain a ledger.Block before calling chain.Chain.AddBlock, since AddBlock expects a ledger.Block, while LedgerState.GetBlock returns a database/models.Block.
Applied to files:
ledger/state.goledger/chainsync_test.goledger/state_test.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: utxorpc tests must pass github.com/blinklabs-io/gouroboros/ledger/common.Block to chain.Chain.AddBlock. They should create database/models.Block only as a storage row, then call models.Block.Decode() to obtain a ledger/common.Block for AddBlock and any ledger/protocol-facing logic.
Applied to files:
ledger/state.goledger/chainsync_test.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.
Applied to files:
ledger/state.goledger/chainsync_test.go
📚 Learning: 2025-10-22T20:18:45.125Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T20:18:45.125Z
Learning: In tests, Chain.AddBlock must be given a gouroboros ledger.Block (not database/models.Block). Use a lightweight MockBlock (as in chain/chain_test.go) or call models.Block.Decode() when starting from stored rows.
Applied to files:
ledger/chainsync_test.go
🪛 GitHub Actions: go-test
ledger/chainsync_test.go
[error] 26-26: "github.com/blinklabs-io/dingo/database" imported and not used
🪛 GitHub Actions: golangci-lint
ledger/state.go
[error] 436-436: golangci-lint: File is not properly formatted (gci)
🪛 GitHub Actions: nilaway
ledger/chainsync_test.go
[error] 26-26: "github.com/blinklabs-io/dingo/database" imported and not used. (from build/test step: nilaway ./... || true)
🪛 GitHub Check: go-test (1.25.x, macos-latest)
ledger/chainsync_test.go
[failure] 26-26:
"github.com/blinklabs-io/dingo/database" imported and not used
🪛 GitHub Check: go-test (1.25.x, ubuntu-latest)
ledger/chainsync_test.go
[failure] 26-26:
"github.com/blinklabs-io/dingo/database" imported and not used
🪛 GitHub Check: lint
ledger/state.go
[failure] 436-436:
File is not properly formatted (gci)
🪛 GitHub Check: nilaway
ledger/chainsync_test.go
[failure] 26-26:
"github.com/blinklabs-io/dingo/database" imported and not used
🔇 Additional comments (3)
ledger/chainsync.go (2)
142-145: LGTM! Dynamic stability window correctly implemented.The replacement of the hard-coded threshold with a call to
calculateStabilityWindow()successfully achieves the PR's objective of deriving the blockfetch slot threshold from protocol parameters.
261-263: LGTM! Genesis hash validation correctly added.The validation ensures that an empty
ShelleyGenesisHashis caught before attempting to decode it, preventing the test failure that was identified in previous reviews.ledger/state.go (1)
439-504: LGTM! Shelley+ stability window calculation is correct.The implementation properly:
- Validates all genesis parameters before use
- Computes the ceiling of 3k/f using correct integer arithmetic
- Guards against overflow and underflow
- Falls back to the default threshold on any validation failure
ledger/chainsync_test.go
Outdated
| "testing" | ||
|
|
||
| "github.com/blinklabs-io/dingo/config/cardano" | ||
| "github.com/blinklabs-io/dingo/database" |
There was a problem hiding this comment.
Remove unused database import.
The database package is imported but never used in this test file. This is causing pipeline failures.
Apply this diff:
"github.com/blinklabs-io/dingo/config/cardano"
- "github.com/blinklabs-io/dingo/database"
"github.com/blinklabs-io/dingo/database/models"
"github.com/blinklabs-io/dingo/ledger/eras"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "github.com/blinklabs-io/dingo/database" | |
| "github.com/blinklabs-io/dingo/config/cardano" | |
| "github.com/blinklabs-io/dingo/database/models" | |
| "github.com/blinklabs-io/dingo/ledger/eras" |
🧰 Tools
🪛 GitHub Actions: go-test
[error] 26-26: "github.com/blinklabs-io/dingo/database" imported and not used
🪛 GitHub Actions: nilaway
[error] 26-26: "github.com/blinklabs-io/dingo/database" imported and not used. (from build/test step: nilaway ./... || true)
🪛 GitHub Check: go-test (1.25.x, macos-latest)
[failure] 26-26:
"github.com/blinklabs-io/dingo/database" imported and not used
🪛 GitHub Check: go-test (1.25.x, ubuntu-latest)
[failure] 26-26:
"github.com/blinklabs-io/dingo/database" imported and not used
🪛 GitHub Check: nilaway
[failure] 26-26:
"github.com/blinklabs-io/dingo/database" imported and not used
🤖 Prompt for AI Agents
In ledger/chainsync_test.go around line 26, the import
"github.com/blinklabs-io/dingo/database" is unused and causing build failures;
remove that import from the file's import block (or reference it if
intentionally needed), then run `go test` to confirm the pipeline error is
resolved.
- Replace hard-coded blockfetchBatchSlotThreshold with dynamic calculation - Use correct security parameters based on current era: * Byron era: K parameter from ByronGenesis.ProtocolConsts.K * Shelley+ eras: SecurityParam from ShelleyGenesis.SecurityParam - Update stability window calculation in both chainsync and validation logic - Calculate stability window as 3k/f for Shelley+ eras, k for Byron era - Ensures blockfetch operations respect protocol-defined stability window Fixes the TODO to calculate slot threshold from protocol params Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
wolf31o2
force-pushed
the
feature/use-stability-window-for-blockfetch
branch
from
8825574 to
c2ab4d2
Compare
Summary
This PR replaces the hard-coded slot threshold for bulk blockfetch operations with the protocol-defined stability window, addressing the TODO comment in the codebase.
Changes
Core Changes
Files Modified
Benefits
Testing
Technical Details
The stability window is calculated differently based on the era:
This ensures that bulk blockfetch operations stop at the appropriate distance from the chain tip, as defined by the protocol's stability guarantees.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Closes #591